Patch dangerfile to only do "use strict" checks on non-es modules #2709
Conversation
ES modules are strict mode by default, so the check should look for use of "import"/"export" to determine if flagging is required.
quantizor
changed the title
Current coverage is 68.35% (diff: 100%)@@ master #2709 diff @@
==========================================
Files 140 140
Lines 5060 5060
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 3459 3459
Misses 1601 1601
Partials 0 0
|
| // based on assumptions from https://github.com/nodejs/node/wiki/ES6-Module-Detection-in-Node | ||
| // basically it needs to contain at least one line with import or export at the | ||
| // beginning of it, followed by a space; any content thereafter doesn't matter | ||
| const esModuleRegex = /^(import|export)\s/g; |
There was a problem hiding this comment.
This will actually also include Flow types imports/exports
There was a problem hiding this comment.
OK is there any indicator that a Flow file is a Flow file?
There was a problem hiding this comment.
Ah OK, I'll send a follow-up PR later today
There was a problem hiding this comment.
@thymikee Hmm so this is somewhat complicated... since files that are typed by Flow also have the @flow comment, it's nearly impossible to differentiate the flow type files from normal ES modules. What do you recommend here, should I just roll back this change?
In general I'm a little confused by the fact that flow uses ES module-like import syntax. Does that mean those files need transpilation via Babel?
There was a problem hiding this comment.
Flow uses import type/export type syntax, so I think you can safely match it.
There was a problem hiding this comment.
It does, but it seems like it's using ES module-like syntax in a commonjs file? Does using flow automatically make a file an es module? If so, that's fine and we don't actually have to change anything.
There was a problem hiding this comment.
I think it doesn't, it's a Flow convenience syntax that is being stripped by babel transform so node module resolver shouldn't be aware of it anyway, but I'm not 100% sure. @cpojer?
…js#2709) ES modules are strict mode by default, so the check should look for use of "import"/"export" to determine if flagging is required.
…js#2709) ES modules are strict mode by default, so the check should look for use of "import"/"export" to determine if flagging is required.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
ES modules are strict mode by default, so the check should look for use of "import"/"export" to determine if flagging is required.
Closes #2708